Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhancement: Detect polymorphic associations in generator #3645

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

Nevelito
Copy link
Contributor

@Nevelito Nevelito commented Feb 6, 2025

Description

Changes:

  • implement detect_polymorphic_associations to resource_ganerator
  • add specs

Fixes # (issue)
#3536

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@Nevelito Nevelito changed the title Chceck potential polymorphic association WIP Detect polymorphic associations in generator Feb 6, 2025
def fields_from_model_tags
tags.each do |name, _|
fields[(remove_last_word_from name).pluralize] = {field: "tags"}
def detect_polymorphic_associations
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method detect_polymorphic_associations has a Cognitive Complexity of 17 (exceeds 5 allowed). Consider refactoring.

Copy link

codeclimate bot commented Feb 6, 2025

Code Climate has analyzed commit d7adb9a and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @Nevelito!

I have a few questions:

lib/generators/avo/resource_generator.rb Outdated Show resolved Hide resolved
lib/generators/avo/resource_generator.rb Outdated Show resolved Hide resolved
lib/generators/avo/resource_generator.rb Outdated Show resolved Hide resolved
@Nevelito Nevelito requested a review from Paul-Bob February 7, 2025 13:52
@Nevelito Nevelito changed the title WIP Detect polymorphic associations in generator enhancement: Detect polymorphic associations in generator Feb 7, 2025
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're getting close to merge this @Nevelito

I let some comments about reusing the code we have instead creating a new method for the polymorphic string constructor.

lib/generators/avo/resource_generator.rb Outdated Show resolved Hide resolved
@Nevelito Nevelito requested a review from Paul-Bob February 11, 2025 09:08
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is looking good @Nevelito

Let's just change the message when types are empty and add the types on the test, otherwise is good to merge.

Thank you for this contribution!

spec/features/avo/generators/resource_generator_spec.rb Outdated Show resolved Hide resolved
lib/generators/avo/resource_generator.rb Outdated Show resolved Hide resolved
@Nevelito
Copy link
Contributor Author

Nevelito commented Feb 11, 2025

I think reduce 3 lines in resource_generator is impossible, we can reduce one here and move first " if " to first line:

fields[name] =
              if association.polymorphic?
                field_with_polymorphic_association(association)
              elsif association.is_a?(ActiveRecord::Reflection::ThroughReflection)
                field_from_through_association(association)
              else
                ::Avo::Mappings::ASSOCIATIONS_MAPPING[association.class]
              end

Of course if we want to keep the code readable

@Paul-Bob
Copy link
Contributor

@Nevelito, I noticed that the generated types aren't constants.

They should be formatted like this: types: [Team, Project, Post, Fish]

Alternatively, types: [::Team, ::Project, ::Post, ::Fish] is also valid.

However, my last suggestion mistakenly converted types: [:Team, :Project, :Post, :Fish] into types: ["Team", "Project", "Post", "Fish"], both of which are incorrect.

@Nevelito
Copy link
Contributor Author

I tried to use constantize witch should fix this issue but it returns types like this

types: [Team(id: integer, name: string, description: text, created_at: datetime, updated_at: datetime, url: string, color: string), Project(id: inte ...

@Paul-Bob
Copy link
Contributor

I tried to use constantize witch should fix this issue but it returns types like this

types: [Team(id: integer, name: string, description: text, created_at: datetime, updated_at: datetime, url: string, color: string), Project(id: inte ...

I know, I don't know a easy way to achieve this, could you do some research on this please?

@Nevelito
Copy link
Contributor Author

I created alias with new inspect function and keep original inspect in function original_inspect, now it works as it should but let me know what you think

@Paul-Bob
Copy link
Contributor

I created alias with new inspect function and keep original inspect in function original_inspect, now it works as it should but let me know what you think

@Nevelito you did that change on the spec/dummy/app/models/application_record.rb which is inside Avo's dummy application. What will happen on user's applications that will not have this monkey patch? Most provably will not work as expected and the types will be rendered as: types: [Team(id: integer, name: string, description: text, created_at: datetime, updated_at: datetime, url: string, color: string), Project(id: inte ... which is not ideal. We want this way of rendering types: types: [Team, Project, Post, Fish] on all the applications

Suggesting this change to all the users is not ideal and monkey patching the ActiveRecord::Base it's also not reasonable.

Let's try to achieve this output: types: [Team, Project, Post, Fish] without monkey-patches if possible

@Nevelito
Copy link
Contributor Author

I am not sure if it is possible, I tried to find something but nothing works, only changing inspect gave us wanted result but I see that we can not make like that because of this is in dummy app. I not sure what can we do now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants